Skip to content
This repository has been archived by the owner on Nov 17, 2023. It is now read-only.

[MXNET-210]give warning for variables with same name in graph visualization #10429

Merged
merged 9 commits into from
Jul 2, 2018
Merged

[MXNET-210]give warning for variables with same name in graph visualization #10429

merged 9 commits into from
Jul 2, 2018

Conversation

roywei
Copy link
Member

@roywei roywei commented Apr 6, 2018 •
edited
Loading

Description

Issue 9898
MXNet allow to use same name for variables, it could cause confusion

Checklist

Essentials

Please feel free to remove inapplicable items for your PR.

  • The PR title starts with MXNET-210
  • Changes are complete (i.e. I finished coding on this PR)
  • All changes have test coverage:
  • Unit tests are added for small changes to verify correctness (e.g. adding a new operator)
  • Nightly tests are added for complicated/long-running ones (e.g. changing distributed kvstore)
  • Build tests will be added for build configuration changes (e.g. adding a new build option with NCCL)
  • Code is well-documented:
  • For user-facing API changes, API doc string has been updated.
  • For new C++ functions in header files, their functionalities and arguments are documented.
  • For new examples, README.md is added to explain the what the example does, the source of the dataset, expected performance on test set and reference to the original paper if applicable
  • Check the API doc at http://mxnet-ci-doc.s3-accelerate.dualstack.amazonaws.com/PR-$PR_ID/$BUILD_ID/index.html
  • To the my best knowledge, examples are either not affected by this change, or have been fixed to be compatible with this change

Changes

  • Check if there are nodes with the same name in graph, gave warning to prevent confusion in visualization

Comments

  • If this change is a backward incompatible change, why must this change be made.
  • Interesting edge cases to note here

@roywei roywei requested a review from szha as a code owner April 6, 2018 00:18
@roywei roywei mentioned this pull request Apr 6, 2018
Closed
sandeep-krishnamurthy
sandeep-krishnamurthy reviewed Apr 9, 2018
View reviewed changes
python/mxnet/visualization.py Outdated
@@ -252,6 +252,10 @@ def plot_network(symbol, title="plot", save_format='pdf', shape=None, node_attrs
shape_dict = dict(zip(interals.list_outputs(), out_shapes))
conf = json.loads(symbol.tojson())
nodes = conf["nodes"]
# check if multiple nodes have the same name
if len(nodes) != len(set([node["name"] for node in nodes])):
logging.warning("There are multiple variables with the same name in your graph, "
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It would good to print the duplicate name in your warning message.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Second this.

sandeep-krishnamurthy
sandeep-krishnamurthy approved these changes Apr 9, 2018
View reviewed changes
Copy link
Contributor

@sandeep-krishnamurthy sandeep-krishnamurthy left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks for the change.

@reminisce - Can you please take a look at this?

@Roshrini
Copy link
Member

@reminisce @eric-haibin-lin Can somebody review and merge if its okay?

reminisce
reminisce reviewed Apr 12, 2018
View reviewed changes
python/mxnet/visualization.py Outdated
@@ -252,6 +252,15 @@ def plot_network(symbol, title="plot", save_format='pdf', shape=None, node_attrs
shape_dict = dict(zip(interals.list_outputs(), out_shapes))
conf = json.loads(symbol.tojson())
nodes = conf["nodes"]
# check if multiple nodes have the same name
if len(nodes) != len(set([node["name"] for node in nodes])):
seen = set()
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

seen_nodes

reminisce
reminisce reviewed Apr 12, 2018
View reviewed changes
python/mxnet/visualization.py Outdated
# check if multiple nodes have the same name
if len(nodes) != len(set([node["name"] for node in nodes])):
seen = set()
seen_add = seen.add
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Why need to define this?

@reminisce
Copy link
Contributor

Could you add a unit test? I think in Python you can assert warnings.

@roywei
Copy link
Member Author

roywei commented Apr 12, 2018

@marcoabreu I am adding a unit test which require graphviz, may I know where shall I configure that in CI for Windows? Thanks

@marcoabreu
Copy link
Contributor

Hello, I'm afraid we're not able to change the Windows environment as of now. For now, please use a try-catch and skip if the import failed.
We got somebody working on this, but I can't give an ETA.

@roywei roywei force-pushed the variable_reuse branch 2 times, most recently from 2d276c0 to 2857a10 Compare April 18, 2018 20:06
@eric-haibin-lin
Copy link
Member

please resolve conflicts

@szha
Copy link
Member

szha commented Jun 28, 2018

@roywei ping

@roywei
Copy link
Member Author

roywei commented Jul 2, 2018

@eric-haibin-lin @szha resolved conflicts, and finally CI passed.

@szha szha merged commit 7c74d1f into apache:master Jul 2, 2018
XinYao1994 pushed a commit to XinYao1994/incubator-mxnet that referenced this pull request Aug 29, 2018
@roywei @szha
[MXNET-210]give warning for variables with same name in graph visuali…
db49dc7 
…zation (apache#10429)

* give warning for variables with same name in graph visualization

* fix line too long

* print repetead node names

* update warning and unit test

* add assert for repeated node

* add graphviz for arm

* update docker install

* skip unittest if graphviz could not be imported

* optimize imports
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Reviewers

@reminisce reminisce reminisce left review comments

@sandeep-krishnamurthy sandeep-krishnamurthy sandeep-krishnamurthy approved these changes

@szha szha Awaiting requested review from szha szha is a code owner

@sergeykolychev sergeykolychev Awaiting requested review from sergeykolychev

@thirdwing thirdwing Awaiting requested review from thirdwing

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
7 participants
@roywei @Roshrini @reminisce @marcoabreu @eric-haibin-lin @szha @sandeep-krishnamurthy